Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gatsby V2 upgrade #8

Merged
merged 48 commits into from
Jan 3, 2019
Merged

Gatsby V2 upgrade #8

merged 48 commits into from
Jan 3, 2019

Conversation

travis-r6s
Copy link
Contributor

@travis-r6s travis-r6s commented Sep 18, 2018

Updated to use Gatsby V2.

This also fixes a number of issues, such as module imports, using sass (gatsby now directly imports the style.scss using the gatsby sass plugin) and has added unique keys (either using index, or post.id) to all .map() methods.

Have tested on my machine, and 'internally' everything works fine.

However, even though I have changed nothing on those components, the menus don't seem to stay expanded on page change anymore. Not sure what that would be?

@travis-r6s
Copy link
Contributor Author

Have also added a couple of scripts copied from the official Gatsby starter, which run ESLint and Prettier.
Following their scripts, this has removed all semicolons from all project files.

@travis-r6s
Copy link
Contributor Author

This should fix issues #6, #5, and #3.

@bradfrost
Copy link
Owner

Hey @thetre97, thanks so much for this PR! I'm just getting back into work after several weeks attending to a family medical emergency. I won't be able to review this straight away, but hopefully I can get to it soon. Thanks!

@travis-r6s
Copy link
Contributor Author

No problem @bradfrost - I'm sorry to hear that, I hope everything is okay.
Look forward to your review.

@jcameronjeff
Copy link

However, even though I have changed nothing on those components, the menus don't seem to stay expanded on page change anymore. Not sure what that would be?

@thetre97 Did you ever figure this out? I've been wrestling with it for days.

@travis-r6s
Copy link
Contributor Author

@jcameronjeff I'm afraid not - I couldn't get it to work either.
As I said, I don't think I changed any of the functionality of the pages, so perhaps it was a v2 upgrade that broke how something used to work?


export class PrimaryNavItem extends Component {
constructor(props) {
super(props)
this.state = { isNavOn: true }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.state = { isNavOn: !window.location.pathname.includes(props.href)}


export class PrimarySubNavItem extends Component {
constructor(props) {
super(props)
this.state = { isSubNavOn: true }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.state = { isSubNavOn: !window.location.pathname.includes(props.url) }

@jess-mann
Copy link

jess-mann commented Dec 22, 2018

@jcameronjeff @thetre97
So, I think the v1 to v2 switch of the react router to the reach router causes this issue. When the route is changed, it causes the components to remount (which calls their constructors and defaults their state). I suggested a hacky solution above that solves this issue without refactoring the code.

@travis-r6s
Copy link
Contributor Author

Thanks @jess-mann - that works great. Have pushed those changes.

@bradfrost
Copy link
Owner

Hey @thetre97 and @jess-mann! Thanks so much for the work on this. I was able to pull down the changes and run gatsby develop, which ran beautifully.

However, running gatsby build seems to throw an error:

screenshot 2019-01-02 09 24 56

error Building static HTML for pages failed

See our docs page on debugging HTML builds for help https://goo.gl/yL9lND

   5 |   constructor(props) {
   6 |     super(props)
>  7 |     this.state = { isNavOn: !window.location.pathname.includes(props.href) }
     |                              ^
   8 |     this.toggleNav = this.toggleNav.bind(this)
   9 |   }
  10 |


  WebpackError: ReferenceError: window is not defined

  - PrimaryNavItem.js:7 new PrimaryNavItem
    lib/src/components/PrimaryNavItem.js:7:30

I'm not exactly sure why window isn't defined, but this issue might have some clues in there. If you would be able to iron that out I'll merge this in. Thanks a million!

@travis-r6s
Copy link
Contributor Author

travis-r6s commented Jan 2, 2019

I believe it because Node doesn't have access to the window object when it is building Gatsby.
Something like this should work? Could perhaps be put better though.

typeof window !== 'undefined' ? !window.location.pathname.includes(props.href) : false

@bradfrost
Copy link
Owner

Nice! I just checked and everything builds and looks great. Thanks again for all of this work!

@bradfrost bradfrost merged commit e8c9b8e into bradfrost:master Jan 3, 2019
@travis-r6s
Copy link
Contributor Author

I believe It should also fix some of the currently open issues?

@travis-r6s travis-r6s deleted the v2 branch May 2, 2019 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants